Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have "stack init" accept a list of sub-directories #1639

Closed
wants to merge 18 commits into from

Conversation

hackeryarn
Copy link
Contributor

@mgsloan Is this the rough behavior referred to in #1593?

After a confirmation I will add list behavior instead of single argument, and some clean up to how the arguments are handled.

where
subDir =
textArgument
(metavar "SUB-DIRECTORIES" <>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be more consistent to just use "DIR".

Also, we might want to call this something like "searchDir", and clarify this in the help - something like "Directories which are searched for cabal files. If none specified, uses only the current directory."

@mgsloan
Copy link
Contributor

mgsloan commented Jan 13, 2016

LGTM, other than the comment.

In general I'd prefer the formatting changes happening in a different commit, but it's no big deal.

@hackeryarn
Copy link
Contributor Author

Ha, I didn't even notice those. I run stylish-haskell and those changes just happened on save. I will back them out before the final commit.

@mgsloan
Copy link
Contributor

mgsloan commented Jan 27, 2016

Pinging @achernyak , how's it going?

@hackeryarn
Copy link
Contributor Author

@mgsloan Sorry for the long delay. Things got a bit crazy at work the last couple weeks. I plan to wrap everything up this weekend with all your suggestions and get this pushed out.

@mgsloan
Copy link
Contributor

mgsloan commented Jan 27, 2016

Sounds good! I'm just taking a look at all the open PRs!

I'll try to get #1674 merged first, as otherwise we may need to resolve some tricky merge conflicts.

@hackeryarn
Copy link
Contributor Author

@mgsloan I rebased the branch to get rid of some of the big style changes present before.

Also, now a list of directories can be passed in.

@mgsloan
Copy link
Contributor

mgsloan commented Feb 5, 2016

Hey, sorry for the review delay. Are you sure this works? To me it looks like it invokes init on each of the subdirectories, which isn't what we want.

@hackeryarn
Copy link
Contributor Author

No problem at all. I must have miss understood the exact ask here. I see two other options:

  1. It only takes one subdirectory to search and runs init on it.
  2. It searches each of the subdirectories for a .cabal file and only runs init on the directories that have a .cabal file. In this scenario not sure if it should search all subdirectories of those directories too.

Let me know which is more aligned with what is expected.

@mgsloan
Copy link
Contributor

mgsloan commented Feb 6, 2016

I think the main thing is that we only want to generate one stack.yaml, and so should only call initProject once.

It searches each of the subdirectories for a .cabal file and only runs init on the directories that have a .cabal file. In this scenario not sure if it should search all subdirectories of those directories too.

Right here, we want to search in the directories listed by the user. Searching in all subdirectories is controlled by includSubdirs

@hackeryarn
Copy link
Contributor Author

Got it, so we are only searching the specified directories and ignoring the subdirectories of the provided directories. Then we run initProject on the first .cabal file we find.

@mgsloan
Copy link
Contributor

mgsloan commented Feb 6, 2016

Well, we're trying to get a list of all the cabal files in the project cabalfps. Then, initProject tries to figure out a configuration that can build them all together.

@harendra-kumar
Copy link
Collaborator

I guess you don't need to change initCmd at all. Just extract the list of directories passed by the user inside initProject, use findCabalFiles on all of them (currently we use it only on CurrDir) and concat the result. The rest should be taken care of by the existing code. It should create a stack.yaml with all the packages in the specified directories.

@hackeryarn
Copy link
Contributor Author

@harendra-kumar Thanks, that makes complete sense. Realy cleared up my understanding on this one.

@hackeryarn
Copy link
Contributor Author

Just tested and I think this version should meet the behaviour described by @harendra-kumar.

@hackeryarn
Copy link
Contributor Author

Sorry, I missed the update to path-io so I ran into some conflicts I didn't notice until after the push. It is all fixed now.

mgsloan and others added 2 commits February 10, 2016 15:56
Fix withUnpackedTarball7z to find name of srcDir after unpacking commercialhaskell#1774
There will be breaking changes in version 1.0.0 that affect some
functions that Stack uses, so it's better to have this upper bound for
now.
@@ -35,13 +35,15 @@ import Network.HTTP.Client.Conduit (HasHttpManager)
import Path
import Path.IO
import Stack.BuildPlan
import Stack.Config (getSnapshots,
makeConcreteResolver)
import Stack.Config (getSnapshots,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is duplicated.

@harendra-kumar
Copy link
Collaborator

Functionally it looks good. I added a few minor comments. Can you address those and squash the changes into a single commit?

Unrelated formatting changes are better done via separate commits. Though its fine for this one.

@hackeryarn
Copy link
Contributor Author

@harendra-kumar Thanks for the detailed review, working on those changes now.

I really need to turn off my stylish-haskell when working on stack. It's the issue where all the style changes are coming from, just auto formatting.

@hackeryarn hackeryarn closed this Feb 11, 2016
@hackeryarn hackeryarn deleted the stack-init branch February 11, 2016 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants